-
Notifications
You must be signed in to change notification settings - Fork 65
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make the request body limit configurable (and increase the default) #1762
base: master
Are you sure you want to change the base?
Conversation
902ef8e
to
d37d1c5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1762 +/- ##
==========================================
- Coverage 90.18% 90.18% -0.01%
==========================================
Files 399 399
Lines 38374 38381 +7
Branches 4293 4294 +1
==========================================
+ Hits 34608 34613 +5
+ Misses 2473 2470 -3
- Partials 1293 1298 +5 ☔ View full report in Codecov by Sentry. |
@hannahbast This seems to be a duplicate of #1089 |
# Conflicts: # src/global/RuntimeParameters.h
@@ -10,6 +10,7 @@ | |||
#include <future> | |||
|
|||
#include "absl/cleanup/cleanup.h" | |||
#include "global/RuntimeParameters.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This header is very expensive to include. It would be nice if the implementation could be moved into a cpp file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for having a look. The HttpServer.h
header is now only included in Server.cpp
so that shouldn't be that much of a problem. The HttpServer unfortunately cannot be moved to a cpp, because the whole class is templated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fine too but I was thinking about adding a helper function in a cpp and using it to call the function indirectly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1-1 with Julian, we tested this quite a bit and it works. Some minor suggestions:
- Error handling when body limit is exceeded (right now the request does nothing and sends an empty reply)
- Increase the default limit from 1 MB to 100 MB
- Move the
#include
ofutil/http/HttpServer.h
fromServer.h
toServer.cpp
- Abbreviate large updates in the returned JSON (this is probably a PR on its own)
- Fix copyright note at the top of
HttpServer.h
Conformance check passed ✅No test result changes. |
|
There is now a runtime parameter
request-body-limit
with a default size of100 MB
. Previously, the implicit limit of Boost'shttp::request
was used, which was1 MB
. When sending large requests viacurl
use a call like this:Sending an
INSERT DATA
currently takes around 40 second per one million triples, which is rather slow because the triples are parsed using the SPARQL parser. They should rather be parser with our RDF parser, which is work for a separate PR. See also #1756 (comment)